-
Notifications
You must be signed in to change notification settings - Fork 185
nginx: obscure internal rewrite URLs #991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
This reverts commit 456de9c.
Just FYI, we're currently thinking that we should wait for .2 to try to merge this PR. We're hoping to start regression testing early next week. Let me know if that doesn't sound right! |
@alxndrsn, we're planning to start regression testing next Tuesday or Wednesday, so I wanted to check about the status of this PR. It's ready for review, right? Looking at the diff count of +138 -2 made me think that it's a big change, but actually only 2 lines of source changed. The rest is tests. That makes me think that there may be time to get it reviewed in time for .2. Who would work best to review this PR? Sadiq? Could you clarify whether the following is still a TODO?
It sounds like you think this change would be helpful, right? Or is that still an open question? |
TODO
Noted while working on #984:
Current setting
proxy_redirect off
prevents nginx from converting URLs likehttp://enketo:8005/v1
to external URLs.This is not a feature that's required. Reverting to the default
proxy_redirect
config will:See: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_redirect
What has been done to verify that this works as intended?
proxy_redirect
directive changes and checked the new tests now fail: buildWhy is this the best possible solution? Were any other approaches considered?
This rule's meaning is subtle.
The change introduces inconsistency, in that redirects are only rewritten if they match the
location
block in which theproxy_pass
is defined.However, it doesn't seem like preserving internal URLs would ever by desirable, so reducing the occasions they can occur is probably helpful.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
No effect.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
next
branch OR only changed documentation/infrastructure (master
is stable and used in production)